[lldb] Swift OS plugin #9839

felipepiovezan · 2025-01-15T21:40:04Z

This PR implements the required functionality for a swift OS plugins that converts Tasks into Threads, allowing LLDB to transparently work with Tasks as if they were threads.
In particular, this addresses all (famous last words?) the outstanding stepping issues in swift async code.

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp

jimingham · 2025-01-15T22:30:32Z

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp

+    }
+
+    // Mask higher bits to avoid conflicts with core thread IDs.
+    uint64_t masked_task_id = 0xdeadbeef00000000 | *task_id;


This will be the visible "TID" for running tasks, right? And what I'd have to type if I wanted to set a "task specific" breakpoint? If so, we might not want it to be deadbeef. That's cute if you know the reference, but otherwise somewhat off-putting...

Oh yeah, I'm actually looking for suggestions here, if we should mask it at all and what to use if we do
Deadbeef was just something to help me grep the logs as I debugged.

jimingham · 2025-01-16T00:45:05Z

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp

+    ThreadSP swift_thread = [&]() -> ThreadSP {
+      // If we already had a thread for this Task in the last stop, re-use it.
+      if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
+          IsOperatingSystemPluginThread(old_thread))


Do you have to handle the case where when you stopped the first time, this Task was backed by "core thread 1" but the next time you stopped it was backed by a different core thread? I don't see where you are changing the backing thread in this case?

I don't see where you are changing the backing thread in this case?

It's just a few lines below, both cases are handled the same way:

swift_thread->SetBackingThread(real_thread);

jimingham · 2025-01-16T00:48:52Z

I note that the only swift specific bits of this are finding the task on a particular thread, and a few checks about whether this should be used that I think belong more properly in the LanguageRuntime that owns the OS plugin...

Would it be possible to make an OperatingSystemTaskOverlay that has all the generic algorithms, and then a swift version that just adds the "how to find the task ID"?. The concept is pretty general, and we could probably mock it with an artificial OS plugin so we could test a lot of this w/o having to involve Swift...

felipepiovezan · 2025-01-24T03:00:02Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeNames.cpp

@@ -302,6 +302,7 @@ class ThreadPlanRunToAddressOnAsyncCtx : public ThreadPlan {
    auto &target = thread.GetProcess()->GetTarget();
    m_funclet_bp = target.CreateBreakpoint(destination_addr, true, false);
    m_funclet_bp->SetBreakpointKind("async-run-to-funclet");
+    m_funclet_bp->SetThreadID(thread.GetID());


to refresh reviewer's memories: this is the thread plan to "run through a task switch and stop when we schedule this same async function again. This plan now sets thread specific breakpoints, since they are now task breakpoints!

If we somehow push this type of plan without having the OS plugin, we are at a risk of breaking in the wrong task. But if the plugin is not there and there are tasks, we are going to have much bigger problems anyway. For example, this code has been associated with a lot of LLDB crashes because it triggers stack plan migration (this is a downstream-only thing that becomes dead code with this patch, and I'll delete it in a follow up PR)

Should be this an inline comment in the source file ?

I would argue that in this case we don't need any comments, because this is just the "natural" thing to do. If anything, it is the old implementation that deserved a comment, as it was essentially making all threads stop.

This plan now sets thread specific breakpoints, since they are now task breakpoints!

I'm not sure I understand what you're saying with this.

I just noticed that the writing was indeed confusing, my bad.
The important thing to note here is that this plan is no longer special, it should work like any other plan: when it sets a breakpoint, it sets a thread specific breakpoint (otherwise it would cause all threads to stop).

A follow up patch will remove this plan in favor of the RunToBreakpointPlan

medismailben · 2025-02-04T17:46:17Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

@@ -91,6 +91,10 @@ const char *SwiftLanguageRuntime::GetStandardLibraryBaseName() {
  return "swiftCore";
 }

+const char *SwiftLanguageRuntime::GetConcurrencyLibraryBaseName() {


nit: Can this return a StringRef or StringLiteral ?

It could, for sure, but it would break with the pattern of the other functions above. We can do a cleanup and change all of them in a separate PR

medismailben

cool stuff! I left some comments / suggestions :)

medismailben · 2025-02-04T17:48:29Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

@@ -131,6 +149,11 @@ static bool IsStaticSwiftRuntime(Module &image) {
  return image.FindFirstSymbolWithNameAndType(swift_reflection_version_sym);
 }

+static bool IsStaticSwiftConcurrency(Module &image) {
+  static const ConstString task_switch_symbol("_swift_task_switch");


This could be:

Suggested change

static const ConstString task_switch_symbol("_swift_task_switch");

static constexpr llvm::StringLiteral task_switch_symbol = "_swift_task_switch";

The rationale here was also just to keep uniformity with the other identical helper functions in this file. But also the call image.FindFirstSymbolWithNameAndType requires a ConstString, so if we have a StringLiteral we would be instantiating a ConstString on every call to this method, as opposed to just once.

medismailben · 2025-02-04T17:49:18Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

@@ -168,6 +191,52 @@ static ModuleSP findRuntime(Process &process, RuntimeKind runtime_kind) {
  return runtime_image;
 }

+ModuleSP SwiftLanguageRuntime::findConcurrencyModule(Process &process) {


nit: casing

Suggested change

ModuleSP SwiftLanguageRuntime::findConcurrencyModule(Process &process) {

ModuleSP SwiftLanguageRuntime::FindConcurrencyModule(Process &process) {

medismailben · 2025-02-04T17:49:34Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.cpp

+}
+
+std::optional<uint32_t>
+SwiftLanguageRuntime::findConcurrencyDebugVersion(Process &process) {


oops, will change!

medismailben · 2025-02-04T17:49:47Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.h

@@ -98,6 +98,16 @@ class SwiftLanguageRuntime : public LanguageRuntime {
  static SwiftLanguageRuntime *Get(lldb::ProcessSP process_sp) {
    return SwiftLanguageRuntime::Get(process_sp.get());
  }
+
+  /// Returns the Module containing the Swift Concurrency runtime, if it exists.
+  static lldb::ModuleSP findConcurrencyModule(Process &process);


medismailben · 2025-02-04T17:50:02Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntime.h

+  /// returned.
+  /// Returns 0 for versions of the module prior to the introduction
+  /// of versioning.
+  static std::optional<uint32_t> findConcurrencyDebugVersion(Process &process);


medismailben · 2025-02-04T17:51:06Z

lldb/source/Plugins/LanguageRuntime/Swift/SwiftLanguageRuntimeNames.cpp

@@ -302,6 +302,7 @@ class ThreadPlanRunToAddressOnAsyncCtx : public ThreadPlan {
    auto &target = thread.GetProcess()->GetTarget();
    m_funclet_bp = target.CreateBreakpoint(destination_addr, true, false);
    m_funclet_bp->SetBreakpointKind("async-run-to-funclet");
+    m_funclet_bp->SetThreadID(thread.GetID());


Should be this an inline comment in the source file ?

medismailben · 2025-02-04T17:53:15Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+    }
+
+    // Mask higher bits to avoid conflicts with core thread IDs.
+    uint64_t masked_task_id = 0x0000000f00000000 | *task_id;


Nice hack :p

To be honest I don't think we need to / should do this, because conflicts are handled without issues... it is just a way to visually know we have a fake thread.

how are the conflicts handled?

It's a couple of lines down, on line 100

medismailben · 2025-02-04T17:55:14Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+      if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
+          IsOperatingSystemPluginThread(old_thread))


First time I'm seeing this syntax :p Don't you want to run the second part of the if condition only if the first one if valid ?

Suggested change

if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);

IsOperatingSystemPluginThread(old_thread))

if ((ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id)) && IsOperatingSystemPluginThread(old_thread))

IsOperatingSystemPluginThread is designed to take a pointer argument, so it knows how to handle nullptrs.

…version

This is here mostly out of an abundance of caution, the default is to use the plugins.

felipepiovezan · 2025-02-04T21:52:29Z

addressed review comments

kastiglione · 2025-02-04T22:28:14Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+  size_t ptr_size = process.GetAddressByteSize();
+  // Offset of a Task ID inside a Task data structure, guaranteed by the ABI.
+  // See Job in swift/RemoteInspection/RuntimeInternals.h.
+  m_task_id_offset = 4 * ptr_size + 4;


is this the job id? if so should the name reflect that? also if so, do we know if we can ignore the task's upper 32 bit identifier?

Based on what I saw in the runtime code, I think it's safe to do so. It is what is being done in other tools anyway.

I'll rename this to "job id"!

kastiglione · 2025-02-04T22:41:38Z

lldb/source/Plugins/OperatingSystem/SwiftTasks/OperatingSystemSwiftTasks.cpp

+    ThreadSP swift_thread = [&]() -> ThreadSP {
+      // If we already had a thread for this Task in the last stop, re-use it.
+      if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
+          IsOperatingSystemPluginThread(old_thread))
+        return old_thread;
+
+      std::string name = llvm::formatv("Swift Task {0:x}", *task_id);
+      llvm::StringRef queue_name = "";
+      return std::make_shared<ThreadMemory>(*m_process, masked_task_id, name,
+                                            queue_name,
+                                            /*register_data_addr*/ 0);
+    }();


The lambda through me off. Is there something I'm missing that it can't be an if/else?

Suggested change

ThreadSP swift_thread = [&]() -> ThreadSP {

// If we already had a thread for this Task in the last stop, re-use it.

if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);

IsOperatingSystemPluginThread(old_thread))

return old_thread;

std::string name = llvm::formatv("Swift Task {0:x}", *task_id);

llvm::StringRef queue_name = "";

return std::make_shared<ThreadMemory>(*m_process, masked_task_id, name,

queue_name,

/*register_data_addr*/ 0);

}();

ThreadSP swift_thread;

// If we already had a thread for this Task in the last stop, re-use it.

if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);

IsOperatingSystemPluginThread(old_thread)) {

swift_thread = old_thread;

} else {

std::string name = llvm::formatv("Swift Task {0:x}", *task_id);

llvm::StringRef queue_name = "";

swift_thread = std::make_shared<ThreadMemory>(*m_process, masked_task_id, name,

queue_name,

/*register_data_addr*/ 0);

}

adrian-prantl reviewed Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

adrian-prantl reviewed Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

adrian-prantl reviewed Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

felipepiovezan commented Jan 15, 2025

View reviewed changes

lldb/source/Plugins/OperatingSystem/Swift/OperatingSystemSwift.cpp Outdated Show resolved Hide resolved

jimingham reviewed Jan 15, 2025

View reviewed changes

jimingham reviewed Jan 16, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch 3 times, most recently from 969ca8a to 8aeb614 Compare January 24, 2025 02:46

felipepiovezan commented Jan 24, 2025

View reviewed changes

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 8aeb614 to 9bd7fa3 Compare January 24, 2025 03:06

felipepiovezan changed the base branch from swift/release/6.1 to stable/20240723 January 24, 2025 17:15

kastiglione mentioned this pull request Jan 29, 2025

[lldb] Introduce Swift "task info" command #9875

Merged

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 9bd7fa3 to 5142ea2 Compare February 4, 2025 15:20

medismailben reviewed Feb 4, 2025

View reviewed changes

felipepiovezan added 3 commits February 4, 2025 13:50

[lldb][swift] Add helper function to find Swift concurrency's module …

457397a

…version

[lldb] Add experimental target option controlling swift OS plugin

ce95a9a

This is here mostly out of an abundance of caution, the default is to use the plugins.

[lldb][swift] Swift OS Plugin

cdc4b54

felipepiovezan force-pushed the felipe/swift_os_plugin_rebase_61 branch from 5142ea2 to cdc4b54 Compare February 4, 2025 21:52

felipepiovezan marked this pull request as ready for review February 4, 2025 21:52

felipepiovezan requested a review from a team as a code owner February 4, 2025 21:52

kastiglione reviewed Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb] Swift OS plugin #9839

[lldb] Swift OS plugin #9839

felipepiovezan commented Jan 15, 2025 •

edited

Loading

jimingham Jan 15, 2025

felipepiovezan Jan 15, 2025

jimingham Jan 16, 2025

felipepiovezan Jan 21, 2025

jimingham commented Jan 16, 2025

felipepiovezan Jan 24, 2025 •

edited

Loading

medismailben Feb 4, 2025

felipepiovezan Feb 4, 2025

kastiglione Feb 4, 2025

felipepiovezan Feb 4, 2025

felipepiovezan Feb 4, 2025

medismailben Feb 4, 2025

felipepiovezan Feb 4, 2025

medismailben left a comment

medismailben Feb 4, 2025

felipepiovezan Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

felipepiovezan Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

medismailben Feb 4, 2025

felipepiovezan Feb 4, 2025

kastiglione Feb 4, 2025

felipepiovezan Feb 4, 2025

medismailben Feb 4, 2025

felipepiovezan Feb 4, 2025 •

edited

Loading

felipepiovezan commented Feb 4, 2025

kastiglione Feb 4, 2025

felipepiovezan Feb 4, 2025

kastiglione Feb 4, 2025

	static const ConstString task_switch_symbol("_swift_task_switch");
	static constexpr llvm::StringLiteral task_switch_symbol = "_swift_task_switch";

	ModuleSP SwiftLanguageRuntime::findConcurrencyModule(Process &process) {
	ModuleSP SwiftLanguageRuntime::FindConcurrencyModule(Process &process) {

		if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
		IsOperatingSystemPluginThread(old_thread))

	if (ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id);
	IsOperatingSystemPluginThread(old_thread))
	if ((ThreadSP old_thread = old_thread_list.FindThreadByID(masked_task_id)) && IsOperatingSystemPluginThread(old_thread))

[lldb] Swift OS plugin #9839

Are you sure you want to change the base?

[lldb] Swift OS plugin #9839

Conversation

felipepiovezan commented Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimingham commented Jan 16, 2025

felipepiovezan Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medismailben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

felipepiovezan commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipepiovezan commented Jan 15, 2025 •

edited

Loading

felipepiovezan Jan 24, 2025 •

edited

Loading

felipepiovezan Feb 4, 2025 •

edited

Loading